-
-
Notifications
You must be signed in to change notification settings - Fork 760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Platform UI - React integration #5011
Conversation
Exciting times! Regarding translation - we will need to be careful that during this transition process we don't delete any existing translated strings (as much as we possibly can). Otherwise we lose all the great work that has been done on crowdin. |
@matmair when this is ready to look at, can you provide a set of instructions to build and run a MVP? |
Those files should be committed - I just removed them for this PR because the UI is empty rn and it makes reading the diff harder without adding any benefit. It is the same as with the language files - we automated that part but at some point they need to be updated and included. |
I would appreciate it if you would not extend your content meaningful after commenting by editing - I mainly read on mails to decide if I need to answer and do not get updates for edits @wolflu05 |
I don't like commiting the build output. As you said, the diff gets really huge, that means also the repo size rapidly grows => it takes an eternity to clone the repo, currently it already needs 40s to clone. I would say that node is required if you develop frontend parts and if you don't like to install it, use the devcontainer. It's a really bad idea if this is the only argument to commit these huge diffs later. And regarding the languages and pseudo languages, you plan to commit them before merge? |
With regards to translation files:
@matmair with this in mind, can you please briefly outline how the translation files are calculated / stored in the new framework? So we can determine how we plan for integration with crowdin. |
@wolflu05 if you see another way to distribute the front end I am open to ideas. The alternative way still has to work for bare metal installs - we would brake the upgrade path for most users otherwise judging from the issues. |
@SchrodingersGat translations work basically the same as they do with the current frontend. Strings are discovered when the frontend-trans task is run, placed in .po files, those are picked up by crowdin, translated, pushed from crowdin to the translation branch and then merged into master before release.
The new translations would be separate, like the app translations
Not sure how much this will be possible - a lot of the interface is underdocumented so there will probably be a lot of new tooltips, menu entries etc.
Uses the same mechanism as the current setup
A lot of strings are also used by the backend so that will not remove a lot of translatable files - a bunch of glue pieces for the current way of translations will be removed tough |
@matmair ok thanks, I'm happy with that. Sounds like we have a way forward at least |
Anything holding us back from merging at this point? |
We can build them and upload a zip file with the point releases. If you want to install from a none point release, require node. I know a lot of softwares that require node for their frontend build (e.g. gitea). And for apt packages, I guess there is already some build step where this could be build. |
Do you like the idea of commiting build files? I already explained a lot of downsides here |
My feeling is that the compiled web files should not be included in the repo. If we run through the different ways to use InvenTree:
|
Compiling the frontend on the server with bare metal and installer means that every user needs to install thousands of external packages on their machine. If only one of those is vulnerable (pretty likely) web proxies or on-device scanners will trigger - effectively making inventree uninstallable in the enterprise/goverment or for responsible sysadmins. |
Why not upload a build output to each point release as a zip file? |
You make some great points here - the security implications in particular I had not considered. I had been considering this front-end code in a similar way to how we currently render static files with I'm happy to move forward with compiled front-end files "baked in". Let's keep this going, this is a super important path forward for the project. |
I see the points with the vulnerabilities, but really, we shouldn't commit the build output. There are a lot of downsides:
How to properly solve this problem:As stated by @SchrodingersGat we have a few targets. My suggesting would be the following: We have to distinguish between development and production. DevelopmentI think for development on the frontend it's not a big deal to install nodejs, you need it anyway. If you only want to do backend, go ahead and use some API client like postman or the integrated one. For devcontainer, its not a big deal the devcontainer contains node anyway. Production
Please, evaluate the downsides, they are big for me. I really don't like to have them in here. I really know we want to move this forward, but once we decide to commit the repo will definitely grow fast and we can't reverse it. |
Maybe due to me being a complete react noob, but is it possible you guys are talking about different things? @wolflu05 you are talking about 200mb worth of files - which directory, specifically, and what purpose do these files serve? @matmair you are talking about 10mb worth of files - which directory, specifically, and what purpose do these files serve? |
I'm talking about the files in |
I would rather have devs download a bigger repo from time to time then most users (especially the ones that have been using this for years when bare metal was the only real deployment option) dealing with node. Manual changes to the update methodology are about the most brutal thing you can do to a sysadmin. @SchrodingersGat I am talking about the real files that are acutally present @wolflu05 with that argument we should stop translations |
I will step back from discussions for a few days as I do not feel that it will be productive in the end. Favouring one deployment method because of the lack of the others will only lead to more support load and if that is the result I'd rather ship the frontend as a plugin or reconsider the rewrite. |
fine with me @SchrodingersGat |
Yeah, also fine with me, merging this now definitely simplifies things e.g. the huge diff in the other PRs. |
Fantastic :) @matmair are there any other changes you want to make here, or happy to merge as-is? |
Yes @SchrodingersGat let's merge |
This PR introduces the base for the new UI based on React. The current UI is referred 'classic' while the new React one is named 'platform' - as it is based on a new UI platform.
The UI itself is de facto empty - it is just a shell to show how the mechanism works. There will be a parallel PR with a first basic interface.
Detailed mechanisms:
Missing:
Fixes #3901 Fixes #2789